composefs/status: resolve rollback entry correctly by chaserhkj · Pull Request #1888 · bootc-dev/bootc

chaserhkj · 2026-01-05T06:13:23Z

Previous implementation had undefined behavior and was coincidentally correct under conditions where no rollback was performed, see #1887

Matches deployment entries in composefs deploy folder that are neither staged nor booted against entires defined in /boot to find out rollback entry.

Fixes #1887

gemini-code-assist

Code Review

This pull request provides a solid fix for correctly identifying the rollback deployment in composefs status. The previous implementation, which assumed any non-booted/non-staged deployment was the rollback entry, was flawed and could lead to incorrect behavior, especially in the presence of stale deployment data. The new approach of cross-referencing deployments with the actual bootloader configuration (both BLS and Grub entries) is much more robust and correctly handles the logic. The changes are well-contained and the logic is sound. I have one suggestion to refactor a loop to be more idiomatic and improve clarity.

crates/lib/src/bootc_composefs/status.rs

Previous implementation had undefined behavior and was coincidentally correct under conditions where no rollback was performed, see bootc-dev#1887 Matches deployment entries in composefs deploy folder that are neither staged nor booted against entires defined in /boot to find out rollback entry. Fixes bootc-dev#1887 Signed-off-by: Chaser Huang <huangkangjing@gmail.com>

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Chaser Huang <huangkangjing@gmail.com>

chaserhkj · 2026-01-08T22:58:28Z

@cgwalters @Johan-Liebert1 Can anyone take a look at this change? I feel this change makes sense even without reproduction of #1887 since current implemented behavior relies on undefined behaviors from the underlying library/syscall ( syscall opendir from Dir::read_dir ) and that could be counted as a bug on itself.

Johan-Liebert1 · 2026-01-09T05:21:59Z

since current implemented behavior relies on undefined behaviors from the underlying library/syscall

Could you please highlight the UB that might occur?

I might be getting this wrong since I'm unable to reproduce #1887, but wouldn't bootc status now fail at
https://github.com/bootc-dev/bootc/pull/1888/changes#diff-0c2404bde8d45d9665ce40004291992c9503391924082c74ef6ef515e1eedf50R728?

chaserhkj · 2026-01-09T06:13:06Z

UB comes from these code that reads the entries from directory /sysroot/state/deploy:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 545 to 549 in ad60763

    
           let deployments = storage 
        
               .physical_root 
        
               .read_dir(STATE_DIR_RELATIVE) 
        
               .with_context(|| format!("Reading sysroot {STATE_DIR_RELATIVE}"))?;

The read_dir function doc describes the order of the returned entry list to be UB.
Later in the same function, this list with undefined order is enumerated through:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 573 to 621 in ad60763

    
           for depl in deployments { 
        
               let depl = depl?; 
        
               let depl_file_name = depl.file_name(); 
        
               let depl_file_name = depl_file_name.to_string_lossy(); 
        
               // read the origin file 
        
               let config = depl 
        
                   .open_dir() 
        
                   .with_context(|| format!("Failed to open {depl_file_name}"))? 
        
                   .read_to_string(format!("{depl_file_name}.origin")) 
        
                   .with_context(|| format!("Reading file {depl_file_name}.origin"))?; 
        
               let ini = tini::Ini::from_string(&config) 
        
                   .with_context(|| format!("Failed to parse file {depl_file_name}.origin as ini"))?; 
        
               let boot_entry = 
        
                   boot_entry_from_composefs_deployment(storage, ini, depl_file_name.to_string()).await?; 
        
               // SAFETY: boot_entry.composefs will always be present 
        
               let boot_type_from_origin = boot_entry.composefs.as_ref().unwrap().boot_type; 
        
               match boot_type { 
        
                   Some(current_type) => { 
        
                       if current_type != boot_type_from_origin { 
        
                           anyhow::bail!("Conflicting boot types") 
        
                       } 
        
                   } 
        
                   None => { 
        
                       boot_type = Some(boot_type_from_origin); 
        
                   } 
        
               }; 
        
               if depl.file_name() == booted_composefs_digest.as_ref() { 
        
                   host.spec.image = boot_entry.image.as_ref().map(|x| x.image.clone()); 
        
                   host.status.booted = Some(boot_entry); 
        
                   continue; 
        
               } 
        
               if let Some(staged_deployment_id) = &staged_deployment_id { 
        
                   if depl_file_name == staged_deployment_id.trim() { 
        
                       host.status.staged = Some(boot_entry); 
        
                       continue; 
        
                   } 
        
               } 
        
               host.status.rollback = Some(boot_entry); 
        
           }

Within this loop, host.status.booted and host.status.staged are both set and early continues the loop, and the last statement catches all fall-through cases and sets host.status.rollback.

But if there are more than two entires in /sysroot/state/deploy that are neither booted nor staged, the loop still goes through all of them, and rollback will eventually settle on the later entry in the list, so what gets resolved to be the rollback image is UB here.

If the UB here picked up an entry that does have a corresponding /boot BLS entry, this won't create a problem as that entry could then be presumed to be the rollback entry and pass all subsequent checks.
If the UB here picked up a stale entry in /sysroot/state/deploy that does not have a corresponding /boot BLS entry, later here in function set_reboot_capable_type1_deployments:

bootc/crates/lib/src/bootc_composefs/status.rs

Lines 444 to 452 in ad60763

    
           for depl in host 
        
               .status 
        
               .staged 
        
               .iter_mut() 
        
               .chain(host.status.rollback.iter_mut()) 
        
               .chain(host.status.other_deployments.iter_mut()) 
        
           { 
        
               let entry = find_bls_entry(&depl.require_composefs()?.verity, &bls_entries)? 
        
                   .ok_or_else(|| anyhow::anyhow!("Entry not found"))?;

find_bls_entry would fail on host.status.rollback due to the missing BLS entry, causing #1887

As for here in my change, that section is trying to find boot entries that are neither the booted entry nor the staged entry, and are in both /sysroot/state/deploy and /boot. The state that there are two entries satisfying this criteria is currently impossible to be in, since bootc under normal state only keeps one booted entry and one rollback entry in /boot, and under staged state only keeps one booted entry, one old rollback entry and one staged entry in /boot

Johan-Liebert1

Thanks for this patch

bootc-bot bot requested a review from cgwalters January 5, 2026 06:13

gemini-code-assist bot reviewed Jan 5, 2026

View reviewed changes

crates/lib/src/bootc_composefs/status.rs Outdated Show resolved Hide resolved

chaserhkj force-pushed the fix-rollback branch from 38de23d to 84c98aa Compare January 8, 2026 22:45

nit: Fix typo and improve readability

ad5d7d5

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Chaser Huang <huangkangjing@gmail.com>

Johan-Liebert1 approved these changes Jan 9, 2026

View reviewed changes

Johan-Liebert1 merged commit 4cb64bb into bootc-dev:main Jan 9, 2026
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

composefs/status: resolve rollback entry correctly#1888

composefs/status: resolve rollback entry correctly#1888
Johan-Liebert1 merged 2 commits intobootc-dev:mainfrom
chaserhkj:fix-rollback

chaserhkj commented Jan 5, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

chaserhkj commented Jan 8, 2026

Uh oh!

Johan-Liebert1 commented Jan 9, 2026

Uh oh!

chaserhkj commented Jan 9, 2026 •

edited

Loading

Uh oh!

Johan-Liebert1 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

chaserhkj commented Jan 5, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

chaserhkj commented Jan 8, 2026

Uh oh!

Johan-Liebert1 commented Jan 9, 2026

Uh oh!

chaserhkj commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Johan-Liebert1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chaserhkj commented Jan 9, 2026 •

edited

Loading